Skip to content

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Oct 16, 2025

Purpose:

This PR bumps the chia_rs dependency to the latest version. The first commit is limited to updating the version and updating usage of it accordingly. The second PR updates the V2Prover class to use the new functionality exposed by chia_rs.

It's recommended to review the commits separately.

Current Behavior:

  • plot strength schedule is used
  • There's no schedule for v2 plot filter
  • partial proofs are expected to be bytes
  • the prover API is assumed to get partial proofs directly from the plot, and then qualities from the partial proof

New Behavior:

  • plot strength schedule has been removed
  • There's a new plot filter schedule for v2 plots
  • partial proofs are list[uint64], and sometimes need to be serialized to form a key for a dictionary
  • the prover API gets qualities from the plot, and only if they pass the difficulty do we request the remaining bits to form the partial proof
  • The V2Prover class can now load v2 plots

@arvidn arvidn requested a review from a team as a code owner October 16, 2025 19:51
@arvidn arvidn added the Added Required label for PR that categorizes merge commit message as "Added" for changelog label Oct 16, 2025
@arvidn arvidn requested a review from almogdepaz October 16, 2025 19:59
@arvidn
Copy link
Contributor Author

arvidn commented Oct 17, 2025

most of the uncovered lines will be covered once we've completed support for v2 plots, and have the test plots and test chains in place

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Oct 17, 2025
@github-actions github-actions bot added coverage-diff and removed merge_conflict Branch has conflicts that prevent merge to main labels Oct 17, 2025
Copy link

coveralls-official bot commented Oct 17, 2025

Pull Request Test Coverage Report for Build 18648956567

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 121 of 176 (68.75%) changed or added relevant lines in 17 files are covered.
  • 43 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.04%) to 90.802%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/solver/solver_api.py 3 4 75.0%
chia/_tests/harvester/test_harvester_api.py 6 11 54.55%
chia/harvester/harvester_api.py 6 15 40.0%
chia/simulator/block_tools.py 9 21 42.86%
chia/plotting/check_plots.py 0 13 0.0%
chia/plotting/prover.py 25 40 62.5%
Files with Coverage Reduction New Missed Lines %
chia/full_node/full_node_api.py 1 86.1%
chia/full_node/full_node.py 1 86.99%
chia/plotting/check_plots.py 1 0.0%
chia/_tests/simulation/test_simulation.py 1 96.47%
chia/farmer/farmer_api.py 2 95.03%
chia/plotting/prover.py 2 81.05%
chia/_tests/core/test_farmer_harvester_rpc.py 2 98.08%
chia/timelord/timelord.py 2 73.03%
chia/server/node_discovery.py 3 80.32%
chia/wallet/wallet_node.py 4 86.32%
Totals Coverage Status
Change from base Build 18601081112: -0.04%
Covered Lines: 102249
Relevant Lines: 112446

💛 - Coveralls

constants: ConsensusConstants,
quality_string: bytes32,
quality_string: bytes,
size: PlotSize,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think in the case of v1 we need to make sure its bytes32

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're suggesting. the quality string is not 32 bytes for v2 plots, but it is for v1 plots. This function supports both, so it has to take bytes.
are you suggesting this function assert len(quality_string) == 32 for the case of v1 plots?


try:
proof = self.solver.solve(request.partial_proof)
proof = self.solver.solve(request.partial_proof, request.plot_id, request.strength, request.size)
Copy link
Contributor

@almogdepaz almogdepaz Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need the plot_id and size for solving ? (i couldn't find any mention of this in the channel)

you can know the size from the partial iirc

i have vague memory of us talking about encoding the strength in the partial did we decide not to ? (i prefer separating it from the partial as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the predictions we made about this API were mostly right. With the concrete implementation, it's just simpler to preserve the partial proof as a list and the strength as a separate variable. This format is not sensitive to size, as opposed to the full proof, which is included in full blocks. That's where we compact the proof and (currently) encode strength as well.

But there's a chance now that we'll lock down the k-size to just a single one, which means we can use the size-field on ProofOfSpace to store the strength for v2 plots, which simplifies the full-proof format.

required_plot_strength = calculate_required_plot_strength(constants, height)
return validate_proof_v2(plot_id, plot_size.size_v2, required_plot_strength, pos.challenge, bytes(pos.proof))
return validate_proof_v2(
plot_id, plot_size.size_v2, constants.PLOT_STRENGTH_INITIAL, pos.challenge, bytes(pos.proof)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are we doing now for required strength ?
its supposed to be dependent on the current state from the chain right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was discussed in a PoS call a few weeks ago. We don't need a pre-determined schedule for increasing strength, since those are soft-forks. We do need a schedule for tightening the plot-filter though, since changing that is a hard fork.

partial_proof: list[uint64] # 16 * k bits blob, k (plot size) can be derived from this
plot_id: bytes32
strength: uint8
size: uint8 # k-size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need size if partial proof is k length ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right that we could (probably) calculate k by len(partial_proof)/16. It would make me a little bit uneasy to not specify the k-size explicitly. Would you prefer omitting k? There's a good chance we'll lock down k to be a constant as well, in which case we don't need it

required_iters: uint64 = calculate_iterations_quality(
self.harvester.constants,
quality_str,
quality.get_quality(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think i would change the var name to quality_proof or something, i think its a bit strange to have quality.get_quality

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to get_string(). In the rust API I called it serialize(), but that only applies to the v2 version. The v1 quality is just the quality string, in serialized form.

if required_iters >= sp_interval_iters:
continue

assert isinstance(plot_info.prover, V2Prover)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be possible we are checking the type before calling
blocking_lookup_v2_partial_proofs

if we want to be defensive then i would put assert isinstance(plot_info.prover, V2Prover) at the start of blocking_lookup_v2_partial_proofs

i think assert isinstance(quality, V2Quality) is redundant especially if you validate the type of the prover

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both of these asserts are primarily to appease mypy who can't tell that the prover is guaranteed to be V2Prover. I think all asserts are defensive and not supposed to be possible by their nature though.

mypy can't tell that just because the prover has the type V2Prover that also the qualities object it returned is necessarily V2Quality

@arvidn arvidn requested a review from almogdepaz October 20, 2025 10:24
Copy link
Contributor

File Coverage Missing Lines
chia/_tests/harvester/test_harvester_api.py 54.5% lines 94-98
chia/harvester/harvester_api.py 40.0% lines 165, 170, 188, 199-200, 202-203, 205-206
chia/plotting/check_plots.py 0.0% lines 10, 12, 18, 180, 208, 212, 214, 217-219, 221-223
chia/plotting/prover.py 62.5% lines 49, 62, 66, 69, 72, 75, 78, 81, 91, 94, 97, 100, 127, 157, 166
chia/simulator/block_tools.py 42.9% lines 1516-1517, 1519-1520, 1524, 1548-1554
chia/solver/solver_api.py 75.0% lines 58
Total Missing Coverage
176 lines 55 lines 68%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Added Required label for PR that categorizes merge commit message as "Added" for changelog coverage-diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants